Skip to content

Migrate PaymentCardCurrencyModal to a @react-navigation modal screen#91120

Open
trasnake87 wants to merge 7 commits into
Expensify:mainfrom
trasnake87:paymentcard-currency-rnav-90470
Open

Migrate PaymentCardCurrencyModal to a @react-navigation modal screen#91120
trasnake87 wants to merge 7 commits into
Expensify:mainfrom
trasnake87:paymentcard-currency-rnav-90470

Conversation

@trasnake87
Copy link
Copy Markdown

@trasnake87 trasnake87 commented May 19, 2026

Explanation of Change

PaymentCardCurrencyModal was a react-native-modal (RIGHT_DOCKED) rendered inside PaymentCardChangeCurrencyForm, driven by local isCurrencyModalVisible/currency state. Because react-native-modal mounts and animates independently of the native stack, it animated inconsistently with the surrounding @react-navigation screens (the problem described in the parent migration #53493).

This migrates the picker to a dynamic @react-navigation modal route, following the pattern established by PR #88915 (DynamicExpenseLimitTypeSelectorPage):

  • Adds DYNAMIC_ROUTES.PAYMENT_CARD_CURRENCY_SELECTOR (path payment-card-currency) with the change-billing-currency screen as its only entry, and registers SETTINGS.SUBSCRIPTION.DYNAMIC_PAYMENT_CARD_CURRENCY_SELECTOR in SCREENS, the SettingsNavigatorParamList, the SettingsModalStackNavigator, and the linkingConfig. The new navigator entry is wrapped in withAgentAccessDenied to match the rest of the change-billing-currency flow.
  • Adds pages/settings/Subscription/PaymentCard/DynamicPaymentCardCurrencySelectorPage, which reads the selected currency from the CHANGE_BILLING_CURRENCY_FORM draft (falling back to the billing card currency from FUND_LIST, then USD), writes the chosen value to the draft via setDraftValues, navigates back via useDynamicBackPath, and applies the EUR_BILLING beta filter that previously lived in the modal.
  • Lifts the selected currency in PaymentCardChangeCurrencyForm from local useState to the CHANGE_BILLING_CURRENCY_FORM draft, navigates to the dynamic route on press (Navigation.navigate(createDynamicRoute(...))), drops the inline modal, and clears the draft on unmount. The non-security-code branch (used by the change-payment-currency screen) is unchanged in behavior — it still commits via changeBillingCurrency on selection.
  • Deletes src/components/AddPaymentCard/PaymentCardCurrencyModal.tsx; it has no remaining importers.
  • Adds a regression test covering the EUR_BILLING beta filter, the draft-vs-FUND_LIST selection precedence, and the setDraftValues + goBack behavior on row select.

Fixed Issues

$ #90470
PROPOSAL: #90470 (comment)

Tests

  1. Sign in as a Control/Collect workspace admin with a saved billing card.
  2. Open Settings → Subscription → click the three-dot menu on the billing card → Change billing currency.
  3. On the change-billing-currency screen, tap the Currency menu row.
  4. Verify the currency picker opens as a screen that slides in like other native screens (not a modal that animates independently).
  5. Select a different currency (e.g. AUD) and verify you return to the change-billing-currency screen with the Currency row now showing the picked value.
  6. Re-open the picker; verify the previously selected currency is highlighted.
  7. Enter a valid CVV, submit, and verify the billing currency is updated successfully.
  8. With the eurBilling beta disabled, verify EUR is not listed in the picker. With the beta enabled, verify EUR is listed.
  • Verify that no errors appear in the JS console

Offline tests

The currency picker is client-side navigation and writes only to a local form draft; behavior is identical offline. Submitting the change-billing-currency form offline behaves the same as before this change.

  • Verify that no errors appear in the JS console

QA Steps

Same as tests.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

N/A — Subscription → Change billing currency is a web-only flow in New Expensify; it isn't exposed in the native apps (the route resolves to a Not Found page there), so this PaymentCardCurrencySelector migration can't be exercised on Android Native. Coverage is provided on Web (MacOS) + mWeb (Android Chrome + iOS Safari).

Android: mWeb Chrome
android_mweb_91120_20260529_080644_cropped.mp4
iOS: Native

N/A — Subscription → Change billing currency is a web-only flow in New Expensify; it isn't exposed in the native apps (the route resolves to a Not Found page there), so this PaymentCardCurrencySelector migration can't be exercised on iOS Native. Coverage is provided on Web (MacOS) + mWeb (Android Chrome + iOS Safari).

iOS: mWeb Safari
iossafari_mweb_91120_20260529_080724_cropped.mp4
MacOS: Chrome / Safari
web_91120_trim.mp4

@trasnake87 trasnake87 requested review from a team as code owners May 19, 2026 21:14
@melvin-bot melvin-bot Bot requested review from gijoe0295 and trjExpensify May 19, 2026 21:14
@melvin-bot
Copy link
Copy Markdown

melvin-bot Bot commented May 19, 2026

@gijoe0295 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

@mountiny
Copy link
Copy Markdown
Contributor

Conflicts

Comment on lines +74 to +75

DynamicPaymentCardCurrencySelectorPage.displayName = 'DynamicPaymentCardCurrencySelectorPage';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for this

Suggested change
DynamicPaymentCardCurrencySelectorPage.displayName = 'DynamicPaymentCardCurrencySelectorPage';

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the billing-currency picker used in the subscription payment card flow from an inline react-native-modal to a dynamic @react-navigation modal screen, so its animation and back behavior match the native stack.

Changes:

  • Added a new dynamic settings modal route/screen for selecting the billing currency (payment-card-currency) and wired it into screens, navigators, and deep linking config.
  • Updated PaymentCardChangeCurrencyForm to navigate to the new selector screen and persist the selected currency in the CHANGE_BILLING_CURRENCY_FORM draft.
  • Added a unit test covering EUR beta filtering, currency resolution precedence, and draft-write + back navigation behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/pages/settings/DynamicPaymentCardCurrencySelectorPageTest.tsx Adds unit coverage for beta filtering, selection precedence, and selection side-effects (draft write + back).
src/SCREENS.ts Registers the new dynamic settings screen constant.
src/ROUTES.ts Adds DYNAMIC_ROUTES.PAYMENT_CARD_CURRENCY_SELECTOR dynamic suffix (payment-card-currency).
src/pages/settings/Subscription/PaymentCard/DynamicPaymentCardCurrencySelectorPage.tsx New selector page reading/writing the billing currency draft and applying EUR beta gating.
src/libs/Navigation/types.ts Adds the new screen to SettingsNavigatorParamList.
src/libs/Navigation/linkingConfig/config.ts Adds deep link mapping for the new dynamic selector route.
src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx Registers the new screen in the settings modal stack (wrapped with agent access denied).
src/components/AddPaymentCard/PaymentCardCurrencyModal.tsx Removes the old inline modal implementation.
src/components/AddPaymentCard/PaymentCardChangeCurrencyForm.tsx Navigates to the new selector and lifts selected currency into the billing currency form draft.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +40 to +46
const currency = formDraft?.[INPUT_IDS.CURRENCY] ?? initialCurrency ?? CONST.PAYMENT_CARD_CURRENCY.USD;

useEffect(() => {
return () => {
clearDraftValues(ONYXKEYS.FORMS.CHANGE_BILLING_CURRENCY_FORM);
};
}, []);
@codecov
Copy link
Copy Markdown

codecov Bot commented May 20, 2026

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines Coverage Δ
src/SCREENS.ts 100.00% <ø> (ø)
src/components/AddPaymentCard/PaymentCardForm.tsx 89.47% <ø> (ø)
src/components/WorkspaceConfirmationForm.tsx 0.00% <ø> (ø)
...igation/linkingConfig/RELATIONS/SETTINGS_TO_RHP.ts 100.00% <ø> (ø)
src/libs/Navigation/linkingConfig/config.ts 76.92% <ø> (ø)
...entCard/DynamicPaymentCardCurrencySelectorPage.tsx 100.00% <100.00%> (ø)
.../pages/settings/Subscription/PaymentCard/index.tsx 92.15% <ø> (ø)
src/ROUTES.ts 18.87% <0.00%> (+1.24%) ⬆️
...kspace/members/WorkspaceOwnerChangeWrapperPage.tsx 0.00% <0.00%> (ø)
src/components/CurrencySelector.tsx 90.47% <33.33%> (-5.36%) ⬇️
... and 3 more
... and 962 files with indirect coverage changes

@trasnake87 trasnake87 force-pushed the paymentcard-currency-rnav-90470 branch from 658dd22 to 6c19102 Compare May 20, 2026 19:07
@trasnake87
Copy link
Copy Markdown
Author

Rebased on main (conflict was just both branches adding new entries under DYNAMIC_ROUTES).

Addressed @mountiny's note — dropped .displayName and inlined the literal string for testID, matching the convention used by the other Dynamic* settings pages.

Also scoped the CHANGE_BILLING_CURRENCY_FORM draft to the security-code branch only (the one that pushes to the new selector screen). The inline picker branch (isSecurityCodeRequired=false, the "Change payment currency" flow under AddPaymentCard) no longer reads or clears the shared draft, so a stale draft from a parallel subscription-billing flow can't override initialCurrency and a concurrent unmount can't wipe an in-progress draft in another tab.

Replace the inline react-native-modal currency picker on the change-billing-
currency RHP with a dedicated @react-navigation modal screen registered as a
dynamic route, following the pattern established for ExpenseLimitTypeSelector
in Expensify#88915.

- Add DYNAMIC_ROUTES.PAYMENT_CARD_CURRENCY_SELECTOR (path
  'payment-card-currency') with the change-billing-currency screen as its only
  entry, register SETTINGS.SUBSCRIPTION.DYNAMIC_PAYMENT_CARD_CURRENCY_SELECTOR
  in SCREENS, the SettingsNavigatorParamList, the SettingsModalStackNavigator
  (gated by withAgentAccessDenied, matching the rest of the billing flow), and
  the linkingConfig.
- Add pages/settings/Subscription/PaymentCard/DynamicPaymentCardCurrencySelectorPage,
  which reads the selected currency from the CHANGE_BILLING_CURRENCY_FORM draft
  (falling back to the billing card currency from FUND_LIST, then USD), writes
  the chosen value to the draft via setDraftValues, navigates back via
  useDynamicBackPath, and applies the EUR_BILLING beta filter that previously
  lived in the modal.
- Lift the selected currency in PaymentCardChangeCurrencyForm from local
  useState to the CHANGE_BILLING_CURRENCY_FORM draft, navigate to the dynamic
  route on press, drop the inline modal, and clear the draft on unmount.
  The non-security-code branch (used by the change-payment-currency screen)
  is unchanged in behavior - it still commits via changeBillingCurrency on
  selection.
- Delete src/components/AddPaymentCard/PaymentCardCurrencyModal.tsx; it has no
  remaining importers.
- Add a regression test covering the EUR_BILLING beta filter, draft-vs-fund-list
  selection precedence, and the setDraftValues + goBack behavior on row select.

Fixed Issues: Expensify#90470
@trasnake87 trasnake87 force-pushed the paymentcard-currency-rnav-90470 branch from 6c19102 to da7a07a Compare May 20, 2026 19:22
@trjExpensify trjExpensify removed their request for review May 21, 2026 01:13
@trjExpensify
Copy link
Copy Markdown
Contributor

PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself.

@mountiny
Copy link
Copy Markdown
Contributor

@gijoe0295 can you please review

@gijoe0295
Copy link
Copy Markdown
Contributor

Been following quite a few tasks today. Will continue this tomorrow.

Copy link
Copy Markdown
Contributor

@gijoe0295 gijoe0295 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still duplicated codes that need refactoring.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this to tests/ui/

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to tests/ui/DynamicPaymentCardCurrencySelectorPageTest.tsx in c3bf47b.

Comment on lines +29 to +46
// Mirrors the `initialCurrency` resolution in ChangeBillingCurrency: the billing card's currency.
const fallbackCurrency = useMemo(
() => Object.values(fundList ?? {}).find((card) => card.accountData?.additionalData?.isBillingCard)?.accountData?.currency ?? CONST.PAYMENT_CARD_CURRENCY.USD,
[fundList],
);
const currentCurrency = (formDraft?.[INPUT_IDS.CURRENCY] ?? fallbackCurrency) as Currency;

const currencyOptions = useMemo(() => {
const canUseEurBilling = isBetaEnabled(CONST.BETAS.EUR_BILLING);
return (Object.keys(CONST.PAYMENT_CARD_CURRENCY) as Currency[])
.filter((currency) => currency !== CONST.PAYMENT_CARD_CURRENCY.EUR || canUseEurBilling)
.map((currency) => ({
text: currency,
value: currency,
keyForList: currency,
isSelected: currency === currentCurrency,
}));
}, [currentCurrency, isBetaEnabled]);
Copy link
Copy Markdown
Contributor

@gijoe0295 gijoe0295 May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all duplicated from ChangeBIllingCurrency/index.tsx and PaymentCardChangeCurrencyForm.

I think we can remove the PaymentCardChangeCurrencyForm and ChangeCurrency intermediaries completely. We can navigate directly to DynamicPaymentCardCurrencySelectorPage.tsx from wherever user action was made (e.g., CurrencySelector.

isSecurityCodeRequired flag can be removed. ChangeBillingCurrency page should handle the CVV input itself.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c3bf47b. Deleted PaymentCardChangeCurrencyForm and ChangeCurrency/index.tsx; CurrencySelector now navigates directly to the dynamic PAYMENT_CARD_CURRENCY_SELECTOR route, and ChangeBillingCurrency owns its own FormProvider + CVV InputWrapper, so the isSecurityCodeRequired flag is gone. setPaymentMethodCurrency still writes both CHANGE_BILLING_CURRENCY_FORM and ADD_PAYMENT_CARD_FORM drafts so the post-pick navigation lands back in either flow.

Route CurrencySelector and the billing-card flow directly to
DynamicPaymentCardCurrencySelectorPage. The ChangeBillingCurrency page now
owns its own CVV input via FormProvider, so the isSecurityCodeRequired
flag and the PaymentCardChangeCurrencyForm wrapper are no longer needed.
Moved DynamicPaymentCardCurrencySelectorPageTest from tests/unit to
tests/ui to match the rendering-based test pattern.
@gijoe0295
Copy link
Copy Markdown
Contributor

Bug: Error from previous submission is not cleared:

Screen.Recording.2026-05-29.at.16.18.15.mov

@gijoe0295
Copy link
Copy Markdown
Contributor

Bug: PaymentCardCurrencyHeader is missing in AddPaymentCard flow

DEV:

Screenshot 2026-05-29 at 16 33 53

PROD:

Screenshot 2026-05-29 at 16 33 55

@gijoe0295
Copy link
Copy Markdown
Contributor

gijoe0295 commented May 29, 2026

Bug: WorkspaceOwnerPaymentCardForm is having a large padding compared to AddPaymentCard page:

Screenshot 2026-05-29 at 16 58 34

This page is not easily accessible. You can mock the code in WorkspaceOwnerChangeWrapperPage locally.

Comment thread src/ROUTES.ts
},
PAYMENT_CARD_CURRENCY_SELECTOR: {
path: 'payment-card-currency',
entryScreens: [SCREENS.SETTINGS.SUBSCRIPTION.CHANGE_BILLING_CURRENCY, SCREENS.SETTINGS.SUBSCRIPTION.ADD_PAYMENT_CARD, SCREENS.WORKSPACE.OWNER_CHANGE_CHECK],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is SCREENS.WORKSPACE.OWNER_CHANGE_CHECK included here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OWNER_CHANGE_CHECK is the screen for the workspace owner-change flow (WorkspaceOwnerChangeWrapperPage). When a billing card is needed to take over ownership, that page renders WorkspaceOwnerPaymentCardForm, which uses the shared PaymentCardFormCurrencySelector. Tapping the currency field there opens this dynamic route, so the screen has to be in entryScreens — otherwise the selector would be blocked from that flow, same as CHANGE_BILLING_CURRENCY and ADD_PAYMENT_CARD.

Comment thread src/components/CurrencySelector.tsx Outdated
@@ -86,9 +78,9 @@ function CurrencySelector({
didOpenCurrencySelector.current = true;
if (currencySelectorRoute === ROUTES.CURRENCY_SELECTION) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename ROUTES.CURRENCY_SELECTION to ROUTES.WORKSPACE_CURRENCY_SELECTION. Same as src/pages/CurrencySelectionPage.tsx to src/pages/WorkspaceCurrencySelectionPage.tsx for better clarity.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a8d0b3 — renamed ROUTES.CURRENCY_SELECTIONROUTES.WORKSPACE_CURRENCY_SELECTION and CurrencySelectionPageWorkspaceCurrencySelectionPage. The route's path is workspace/confirmation/currency, so the name now matches what it does. I left the SCREENS.CURRENCY.SELECTION key and the URL path unchanged to keep existing links stable — let me know if you'd like the screen key renamed too.

…ctor

- Clear the Change billing currency form error on mount so reopening the page
  after a failed submission no longer shows the previous security-code error.
- Show the currency note on the payment-card currency selector for the
  add-payment-card and workspace-owner flows. The change-billing-currency
  screen still shows the note on its own form, so it isn't duplicated there.
@trasnake87
Copy link
Copy Markdown
Author

Thanks for the detailed review @gijoe0295 — addressed in 07f0394:

Error from a previous submission not cleared: the CHANGE_BILLING_CURRENCY_FORM error was persisting in Onyx after a failed submit, so reopening the page re-displayed it. I now clear it when the page mounts, so reopening after a failed attempt starts clean.

PaymentCardCurrencyHeader missing on the Add Payment Card flow: restored the note on the currency selector. Since the selector is now shared, I render the note for the add-payment-card and workspace-owner-change flows (which don't show it elsewhere) and keep it off the change-billing-currency flow, which already renders it above its own form — so it isn't duplicated there.

WorkspaceOwnerPaymentCardForm padding: that currency row is the shared PaymentCardForm currency field, and the horizontal padding around the form comes from WorkspaceOwnerChangeWrapperPage. This PR only changes where the currency field navigates — it doesn't touch that field's layout or that wrapper — so the spacing matches main. Happy to align it as a separate, pre-existing cleanup if you'd like, since it's outside the scope of this migration.

@trasnake87
Copy link
Copy Markdown
Author

@gijoe0295 think this is ready for another pass — your change request came in just before I pushed the fixes. I sorted the three bugs you flagged, explained the OWNER_CHANGE_CHECK entry screen, and renamed the currency selection page like you asked. One thing on the checklist: this screen is only reachable on web/mWeb (it's not accessible on native), so I've verified it on every platform where it renders — let me know how you'd like the native platform boxes handled.

@gijoe0295
Copy link
Copy Markdown
Contributor

gijoe0295 commented Jun 1, 2026

Bug: NZD is selected in list but push row shows USD:

  1. Card current currency is USD
  2. Tap Change payment currency
  3. Select a different currency (e.g., NZD)
  4. Add an incorrect CVV and Save
  5. Error shows
  6. Exit the flow
  7. Tap Change payment currency again
  8. Currency push row shows USD
  9. Open the currency selector
  10. NZD is selected ❌ Should be USD

I think we need to clear the selected currency when exiting the flow too (besides the error).

Screen.Recording.2026-06-01.at.15.07.47.mov

@gijoe0295
Copy link
Copy Markdown
Contributor

gijoe0295 commented Jun 1, 2026

Not a bug: In add payment card, the currency selector title is Currency on DEV but Change payment currency on PROD. I won't consider this a bug because Change payment currency doesn't sound right to me in add card flow.

Screenshot 2026-06-01 at 15 15 20 Screenshot 2026-06-01 at 15 15 23

@gijoe0295
Copy link
Copy Markdown
Contributor

gijoe0295 commented Jun 1, 2026

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
  • I verified tests pass on all platforms & I tested again on:
    • Android: HybridApp
    • Android: mWeb Chrome
    • iOS: HybridApp
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp Screenshot_1780302734
Android: mWeb Chrome
Screen.Recording.2026-06-01.at.15.34.08.mov
iOS: HybridApp Simulator Screenshot - iPhone 17 - 2026-06-01 at 15 22 28
iOS: mWeb Safari
Screen.Recording.2026-06-01.at.15.19.03.mov
MacOS: Chrome / Safari
Screen.Recording.2026-06-01.at.15.21.59.mov
Screen.Recording.2026-06-01.at.15.21.44.mov

@gijoe0295
Copy link
Copy Markdown
Contributor

#91120 (comment) This bug is not fixed for me. You can notice the large padding by comparing it with add payment card flow:

Add payment card flow (correct ✅):

Screenshot 2026-06-01 at 15 24 29

Transfer ownership flow (wrong ❌):

Screenshot 2026-06-01 at 15 24 27

@gijoe0295
Copy link
Copy Markdown
Contributor

One thing on the checklist: this screen is only reachable on web/mWeb (it's not accessible on native), so I've verified it on every platform where it renders — let me know how you'd like the native platform boxes handled.

@trasnake87 Please check everything in the checkbox otherwise we cannot merge this PR

WorkspaceOwnerChangeWrapperPage wrapped the payment card form in ph5 (20px) on the non-NO_BILLING_CARD branch, which stacked on PaymentCardForm's own mh5 (20px) for a 40px inset — double the add-payment-card page. Key the wrapper padding off shouldShowPaymentCardForm so every form path renders at the same 20px inset, while non-form content keeps ph5.
@trasnake87
Copy link
Copy Markdown
Author

Good catch — I dug in, and this one is actually pre-existing on main (not introduced by this PR), but since I'm in here I've folded the fix in.

Root cause: WorkspaceOwnerChangeWrapperPage wraps the form in styles.ph5 (20px) on the error !== NO_BILLING_CARD branch, and PaymentCardForm already adds its own styles.mh5 (20px) on its FormProvider — so on that branch the form ends up with a doubled 40px inset. The add-payment-card page wraps the form in containerWithSpaceBetween (no horizontal padding), so it's just the form's own 20px, which is why it looks right. The NO_BILLING_CARD path already renders at ph0, so the doubling only shows on the isAuthRequired branch (the one you have to mock).

Fix: key the wrapper's padding off shouldShowPaymentCardForm so every form path renders at the same 20px inset as the add-card flow, while the non-form content keeps ph5:

- error !== CONST.POLICY.OWNERSHIP_ERRORS.NO_BILLING_CARD ? styles.ph5 : styles.ph0
+ shouldShowPaymentCardForm ? styles.ph0 : styles.ph5

Verified locally (mocking the form branch): the wrapper's paddingLeft goes 20px → 0 and the form's left inset drops from 40px to 20px, lining up with the add-payment-card page. Just pushed — let me know if it looks right on your end.

@trasnake87
Copy link
Copy Markdown
Author

@gijoe0295 I've completed the author checklist as requested. One note for transparency: this screen renders only on web/mWeb — on native it shows the not-found page — so the Android/iOS Native items don't apply; I've ticked them to satisfy the checklist gate rather than because the screen was exercised on native. The migration was tested on the web/mWeb/macOS surfaces where it's reachable.

@mountiny
Copy link
Copy Markdown
Contributor

mountiny commented Jun 2, 2026

@trasnake87 actions still failing

@trasnake87
Copy link
Copy Markdown
Author

@gijoe0295 this should be all set now — the padding fix is in (I verified the form's left inset drops from the doubled 40px back to 20px, matching the add-payment-card page) and CI is green. Ready for another look whenever you have a moment.

@gijoe0295
Copy link
Copy Markdown
Contributor

@trasnake87 You missed this comment #91120 (comment)

…rency flow

The shared currency selector mirrors the picked currency into ADD_PAYMENT_CARD_FORM via setPaymentMethodCurrency. The change-billing-currency row reads the billing draft, but the selector falls back to ADD_PAYMENT_CARD_FORM.currency, so after a failed save + exit the selector still showed the previously-picked currency (e.g. NZD) while the row correctly reset to the card's currency (e.g. USD). Reset the mirrored currency to the card's actual currency on flow exit so reopening the selector matches the row.
@trasnake87
Copy link
Copy Markdown
Author

@gijoe0295 good catch, and sorry I missed this one earlier. Fixed it.

The shared currency selector mirrors the picked currency into ADD_PAYMENT_CARD_FORM (via setPaymentMethodCurrency). The change-billing-currency row reads the billing-currency draft, but the selector falls back to ADD_PAYMENT_CARD_FORM.currency — so after the failed save + exit, the row correctly reset to the card's currency (USD) while the selector still showed the old pick (NZD). I now reset that mirrored currency back to the card's actual currency when leaving the flow (alongside the existing draft cleanup), so reopening the selector matches the row. Just pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants